Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite wasm-bindgen with updated interface types proposal #1882

Merged
merged 36 commits into from
Dec 3, 2019

Conversation

alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Nov 27, 2019

This commit is a pretty large scale rewrite of the internals of wasm-bindgen. No user-facing changes are expected as a result of this PR, but due to the scale of changes here it's likely inevitable that at least something will break. I'm hoping to get more testing in though before landing!

The purpose of this PR is to update wasm-bindgen to the current state of the interface types proposal. The wasm-bindgen tool was last updated when it was still called "WebIDL bindings" so it's been awhile! All support is now based on https://github.com/bytecodealliance/wasm-interface-types which defines parsers/binary format/writers/etc for wasm-interface types.

This is a pretty massive PR and unfortunately can't really be split up any more afaik. I don't really expect realistic review of all the code here (or commits), but some high-level changes are:

  • Interface types now consists of a set of "adapter functions". The IR in wasm-bindgen is modeled the same way not.
  • Each adapter function has a list of instructions, and these instructions work at a higher level than wasm itself, for example with strings.
  • The wasm-bindgen tool has a suite of instructions which are specific to it and not present in the standard. (like before with webidl bindings)
  • The anyref/multi-value transformations are now greatly simplified. They're simply "optimization passes" over adapter functions, removing instructions that are otherwise present. This way we don't have to juggle so much all over the place, and instructions always have the same meaning.

TODO:

  • re-add optimization to skip shim functions in JS
  • test raytrace example still works

Follow-up work

  • Add tests about the output of the wasm-interface types section from Rust
  • Add tests which consume wasm interface types and produce a JS polyfill

@alexcrichton
Copy link
Contributor Author

Note that this change isn't quite ready to land yet, but I wanted to start getting some CI feedback to see how this fares. Additionally I would like to greatly expand the test coverage of the anyref pass, multivalue pass, and support for interface types. I plan on doing all that before landing this.

(func $dealloc (export "__wbindgen_anyref_table_dealloc") (param i32))
)

(; CHECK-ALL:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Maybe worth pulling our simple filecheck implementation into a reusable crate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been tweaking this as I've gone along for various crates, but it's starting to reach a steady state, yeah, so we should probably look into extracting it soon!

crates/cli-support/src/anyref.rs Outdated Show resolved Hide resolved
crates/cli-support/src/anyref.rs Outdated Show resolved Hide resolved
crates/cli-support/src/js/binding.rs Outdated Show resolved Hide resolved
crates/cli-support/src/js/binding.rs Show resolved Hide resolved
crates/cli-support/src/js/mod.rs Outdated Show resolved Hide resolved
// which calls an imported function.
//
// FIXME(#1872) handle this
// if let Some(Instruction::StoreRetptr { .. }) = instructions.last() {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something we need to handle before merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah this was never exercised or supported before, so I don't think we need to handle this. Once we start writing more wasm-interface-types tests, though, I think we'll want to fill this out pretty shortly afterwards.

crates/cli-support/src/wit/incoming.rs Outdated Show resolved Hide resolved
crates/cli-support/src/wit/mod.rs Outdated Show resolved Hide resolved
(func $foo (export "foo") (param i32))
)

(; CHECK-ALL:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this 👍

This commit removes all the magical names flying around in the JS
generation for managing anyref items and anyref tables. Instead IDs are
set during the anyref pass and these are propagated to the end. This
also deletes references to GC'ing code in the anyref module now that
they've been verified to no longer be needed.
Leave a comment though indicating how it should be reenabled if anyone
hits the error.
Also remove dead code for translating incoming slices since they're not
supported yet in the trait implementations anyway.
Re-enable it for all imported functions, and then selectively disable it
once an intrinsic is called.
@alexcrichton
Copy link
Contributor Author

Ok thanks for the review @fitzgen!

I think I've taken care of everything now, so I'm going to merge this and incrementally improve support for wasm interface types in subsequent PRs, hopefully as a bit more digestable.

@alexcrichton alexcrichton merged commit 8e56cda into rustwasm:master Dec 3, 2019
@alexcrichton alexcrichton deleted the wasm-interface-types branch December 3, 2019 17:16
alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this pull request Dec 3, 2019
alexcrichton added a commit that referenced this pull request Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants